Skip to content

Conversation

@pdgendt
Copy link
Contributor

@pdgendt pdgendt commented Nov 7, 2024

Introduction

As the codebase and contributions keep growing, so does all the python tooling.
This PR is intended to make python scripts more uniform and safe.

Proposed change

Add ruff as a linter and formatter for python files, and verify (new) files in CI.

Detailed RFC

Some hightlights:

  • ruff is a drop-in replacement for many popular python tools, and it is super fast.
  • The formatter output is PEP8 compliant and produces nearly identical output to black
  • Can fix issues for you
  • ...

Current enabled linter rules:

Many more can be added/enabled but this is a solid baseline.

Concerns

Existing files do not comply, so they are explicitly excluded. Ideally these are gradually converted to comply.

$ ruff check
Found 3708 errors.

$ ruff format --check
379 files would be reformatted, 34 files already formatted

Formatting is obviously opinionated, but I do think for python we should follow the popular kids, which are black and ruff.

@pdgendt pdgendt added the RFC Request For Comments: want input from the community label Nov 7, 2024
@pdgendt pdgendt requested a review from carlescufi as a code owner November 7, 2024 12:37
@pdgendt pdgendt force-pushed the ruff branch 3 times, most recently from 80df49f to 80f2713 Compare November 7, 2024 13:18
@nordicjm
Copy link
Contributor

nordicjm commented Nov 7, 2024

Why add exclusions? If someone adds a new file e.g. a runner from an external source, they would have to fix the file, but if an existing runner which does not comply is excluded, that can be updated forever going forwards without having to comply? Or is the idea that new exclusions would be added, in which case what would be the point of having the check if everything is excluded?

@pdgendt
Copy link
Contributor Author

pdgendt commented Nov 7, 2024

Why add exclusions? If someone adds a new file e.g. a runner from an external source, they would have to fix the file, but if an existing runner which does not comply is excluded, that can be updated forever going forwards without having to comply? Or is the idea that new exclusions would be added, in which case what would be the point of having the check if everything is excluded?

This is up for debate of course, but the idea is to force new files to comply, while we gradually convert the existing ones. It could be brought up during a review to update a file you are touching to comply.

@nordicjm
Copy link
Contributor

nordicjm commented Nov 7, 2024

Why add exclusions? If someone adds a new file e.g. a runner from an external source, they would have to fix the file, but if an existing runner which does not comply is excluded, that can be updated forever going forwards without having to comply? Or is the idea that new exclusions would be added, in which case what would be the point of having the check if everything is excluded?

This is up for debate of course, but the idea is to force new files to comply, while we gradually convert the existing ones. It could be brought up during a review to update a file you are touching to comply.

This is something I asked about before because one of the files added from an external source, a uf2 one if I remember right, had some oddities about it, and someone said the file shouldn't be fixed because whilst it was being added to zephyr, it was not a zephyr file, and you then lose the ability to diff etc.

@pdgendt
Copy link
Contributor Author

pdgendt commented Nov 7, 2024

This is something I asked about before because one of the files added from an external source, a uf2 one if I remember right, had some oddities about it, and someone said the file shouldn't be fixed because whilst it was being added to zephyr, it was not a zephyr file, and you then lose the ability to diff etc.

I think this can be an exception, but that's what the exclude lists are for.

@pdgendt pdgendt requested review from tejlmand and removed request for KamilxPaszkiet November 7, 2024 16:01
@hakehuang
Copy link
Contributor

  • Can fix issues for you

can we use ruff to fix the issues in this PR?

Add a compliance test using ruff, for both linting and formatting of
newly added python files.

Signed-off-by: Pieter De Gendt <[email protected]>
@pdgendt
Copy link
Contributor Author

pdgendt commented Nov 18, 2024

Update:

  • Ignore SIM108 to allow if-else blocks instead of forcing ternary operator

@fabiobaltieri
Copy link
Member

@pdgendt where do you want to go from here? It's titled and tagged as RFC but arguably good to go. Maybe arch wg? (though it's not really an architectural change but seems like any change with wide impact goes there these days)

@pdgendt
Copy link
Contributor Author

pdgendt commented Nov 18, 2024

I was thinking the same thing, wanted to bring this up during some workgroup meeting. Could be for arch or process? Maybe we need a tooling working group?

CC @carlescufi @nashif any suggestions?

@fabiobaltieri
Copy link
Member

I think process would be the "correct-er" one but if you want visibility arch is the one to go, I suggest bringing it up there.

@fabiobaltieri fabiobaltieri added the Architecture Review Discussion in the Architecture WG required label Nov 19, 2024
@nashif
Copy link
Member

nashif commented Nov 19, 2024

are we going to keep pylint?

@pdgendt
Copy link
Contributor Author

pdgendt commented Nov 19, 2024

are we going to keep pylint?

There's no conflict between these two, there is some overlap but I haven't added any of the PL rules in this PR.

We might favor enabling the PL rules from ruff and replace pylint with an actual type checker. But that's outside of the scope of this PR.

@nashif
Copy link
Member

nashif commented Nov 19, 2024

There's no conflict between these two, there is some overlap but I haven't added any of the PL rules in this PR.

We might favor enabling the PL rules from ruff and replace pylint with an actual type checker. But that's outside of the scope of this PR.

but it is suboptimal if we have two linters doing python, at leasr from a developer experience. For example, if there is a violation that is detected by both, will we get two warnings?

@pdgendt
Copy link
Contributor Author

pdgendt commented Nov 19, 2024

but it is suboptimal if we have two linters doing python, at leasr from a developer experience. For example, if there is a violation that is detected by both, will we get two warnings?

Presumably, yes. But I haven't looked into the overlaps of the current configurations, this can be optimized on the go.

As you can see from the excludes file, there are a lot of linter issues that pylint hasn't reported on.

@fabiobaltieri
Copy link
Member

I think that before doing this (or at least as part of this) we should at least batch fix the files that are obviously not imported and meant to track an external project, I guess anything under arch, doc, samples, script, tests...

@pdgendt
Copy link
Contributor Author

pdgendt commented Nov 19, 2024

I think that before doing this (or at least as part of this) we should at least batch fix the files that are obviously not imported and meant to track an external project, I guess anything under arch, doc, samples, script, tests...

I was going to do it the other way around, this PR is to prevent any new issues from this point forward.

@fabiobaltieri
Copy link
Member

I was going to do it the other way around, this PR is to prevent any new issues from this point forward.

Yup that makes sense.

@fabiobaltieri
Copy link
Member

  • summarize the issue: linter+formatter check for any new python file
  • @nashif: asks how this behaves with the current files (out of compliance), current files/exclusions woul
    d not trigger
  • @pdgendt: most of these can be fixed esily, will do some folowup to look into this, the tool has an option
    to change the file inline and also do only "safe" changes
  • no conerns on trying this for new files

@fabiobaltieri fabiobaltieri removed the Architecture Review Discussion in the Architecture WG required label Nov 19, 2024
@nashif nashif merged commit 1be5c15 into zephyrproject-rtos:main Nov 19, 2024
30 of 31 checks passed
@pdgendt pdgendt deleted the ruff branch November 20, 2024 07:36
@andyross
Copy link
Contributor

FYI: this isn't running only for new files, as it seems like people were expecting. It's now running on all changes to any python file, to, um hilarious effect, see #81154

@kartben
Copy link
Contributor

kartben commented Nov 27, 2024

FYI: this isn't running only for new files, as it seems like people were expecting. It's now running on all changes to any python file, to, um hilarious effect, see #81154

As noted in the linked issue, you moved existing files so updating the ruff-excludes entries accordingly should bring you back on track.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: Coding Guidelines Coding guidelines and style area: Continuous Integration RFC Request For Comments: want input from the community

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

10 participants